feat: consolidate 2FA settings into User Settings page by DawoudIO · Pull Request #8210 · ChurchCRM/CRM

DawoudIO · 2026-03-07T17:31:01Z

Summary

A full cleanup of the 2FA settings architecture. The core idea: 2FA is a user security feature — every user should be able to self-enroll, and the system should handle all infrastructure automatically.

Before: 2FA required admins to manually configure an encryption key in Config.php, enable bEnable2FA to allow enrollment, and a separate bRequire2FA to mandate it. Enabling bRequire2FA hard-blocked all unenrolled users with "Invalid login or password".

After: The system auto-generates the encryption key on first boot (stored in DB, never exposed), bEnable2FA is removed entirely (enrollment is always available to all users), and bRequire2FA is the only admin control. Enabling it lets users log in and redirects them to enrollment on every request until they complete setup.

Changes

Core Authentication & Config

File	Change
`LoadConfigs.php`	Auto-generate `sTwoFASecretKey` unconditionally on boot — DB+ORM fully initialized before generation, persists immediately via Propel
`SystemConfig.php`	Remove `bEnable2FA`; remove Two-Factor Authentication settings category; add `password` type mapping
`LocalAuthentication.php`	Remove `getIsTwoFactorAuthSupported()`; 2FA prompt driven by enrollment status alone; `bRequire2FA` allows login then redirects to `/v2/user/current/manage2fa` on every request until enrolled
`AdminService.php`	Remove unreachable missing-key warning block
`UserService.php`	Remove `bEnable2FA` and `sTwoFASecretKey` from Quick Settings panel

API & Routes

File	Change
`admin/routes/api/system/system-config.php`	GET returns `{value:''}` for password-type configs; SET no-ops on empty password value
`v2/routes/user-current.php`	`manage2fa()` guard removed; unused imports cleaned up
`v2/templates/user/unsupported-2fa.php`	Deleted — enrollment is always supported

UI

File	Change
`Header.php`	"Manage Two-Factor Authentication" always shown in user dropdown; `LocalAuthentication` import removed
`admin/views/users.php`	Removed Total Logins column; 2FA column always visible with `badge-success`/`badge-secondary` badges; Failed Logins badge only when >0; dropdown right-aligned
`system-settings-panel.js`	`password` type never fetches/displays value; blank with "Leave blank to keep existing" placeholder
`SettingsUser.php`	Cancel button navigates to `/admin/system/users`

Database & Docs

File	Change
`src/mysql/upgrade/7.0.2-2fa.sql`	New — removes `bEnable2FA` from `config_cfg`
`src/mysql/upgrade.json`	Registers 7.0.2-2fa.sql in upgrade chain
`docs: secret-keys.md`	Rewritten as "Two-Factor Authentication" — covers auto-generated key, self-enrollment steps, `bRequire2FA` forced enrollment flow
`docs: faqs.md`	Removed obsolete "Secret Keys missing from Config.php" FAQ entry
`docs: administration/index.md`	Updated link label and description
`wiki: _Sidebar.md`	Updated link label to "Two-Factor Authentication (2FA)"

Test plan

🤖 Generated with Claude Code

Move all two-factor authentication configuration out of the legacy System Settings page and into the admin User Settings panel at /admin/system/users, where it is more contextually appropriate. Security improvements: - Password-type config values are never returned from the API GET endpoint (returns empty string instead) - API SET endpoint guards against overwriting password config with an empty value - Password fields in the settings panel never fetch or display their current value — always rendered blank with "leave blank to keep existing" placeholder - sTwoFASecretKey gets a "Generate" button to fill a fresh 64-char hex key without ever exposing the existing one Other fixes: - getIsTwoFactorAuthSupported() and system warning now also trigger when bRequire2FA is set (not only bEnable2FA) - mapConfigTypeToSettingType() now correctly maps 'password' type Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The panel was titled "User Settings" while the toggle button reads "Quick Settings" — update the panel title to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR consolidates Two-Factor Authentication (2FA) settings into the User Settings admin panel (/admin/system/users), removing the dedicated "Two-Factor Authentication" tab from the legacy System Settings page. It adds security safeguards preventing password-type config values from being exposed via the API, adds a "Generate" button for creating the 2FA encryption key, and fixes two bugs: getIsTwoFactorAuthSupported() now also returns true when 2FA is required (not just when it's enabled), and mapConfigTypeToSettingType() now correctly maps password type.

Changes:

Password-type config values are now protected in the GET API (returns {value: ''}) and guarded against empty-value overwrites in the POST API
The settings panel JS is updated to never fetch/display password values and to generate random secret keys on demand
2FA-related logic (getIsTwoFactorAuthSupported, secrets warning) is updated to trigger on bRequire2FA in addition to bEnable2FA

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`webpack/system-settings-panel.js`	Adds `password` type rendering (blank, no value fetch, generate button) and wires up the Generate button handler with a `crypto.getRandomValues` hex generator
`src/admin/routes/api/system/system-config.php`	Adds password-type guard on GET (returns empty) and POST (no-op on empty value)
`src/ChurchCRM/dto/SystemConfig.php`	Removes 2FA category from legacy settings, adds `generate` flag for `sTwoFASecretKey`, maps `password` config type to settings panel
`src/ChurchCRM/Service/UserService.php`	Adds `sTwoFASecretKey` to the user settings config list
`src/ChurchCRM/Service/AdminService.php`	Warning for missing secrets now also triggers when `bRequire2FA` is set
`src/ChurchCRM/Authentication/AuthenticationProviders/LocalAuthentication.php`	`getIsTwoFactorAuthSupported()` now returns true when `bRequire2FA` is set

Copilot · 2026-03-07T17:39:50Z

src/admin/routes/api/system/system-config.php


-    return SlimUtils::renderJSON($response, ['value' => SystemConfig::getValue($configName)]);
+    // Never overwrite a password with an empty value
+    if (SystemConfig::getConfigItem($configName)->getType() === 'password' && empty($value)) {


Same issue as in getConfigValueByNameAPI: SystemConfig::getConfigItem($configName) may return null for an unknown config name (PHP accesses a non-existent array key), and calling ->getType() on null will produce a fatal PHP error. A null check should be added before dereferencing the result.

Copilot · 2026-03-07T17:39:50Z

src/admin/routes/api/system/system-config.php

 function getConfigValueByNameAPI(Request $request, Response $response, array $args): Response
 {
-    return SlimUtils::renderJSON($response, ['value' => SystemConfig::getValue($args['configName'])]);
+    $configName = $args['configName'];
+    // Never return password values to the browser
+    if (SystemConfig::getConfigItem($configName)->getType() === 'password') {
+        return SlimUtils::renderJSON($response, ['value' => '']);
+    }
+
+    return SlimUtils::renderJSON($response, ['value' => SystemConfig::getValue($configName)]);
 }

 function setConfigValueByNameAPI(Request $request, Response $response, array $args): Response
 {
    $configName = $args['configName'];
    $input = $request->getParsedBody();
-    SystemConfig::setValue($configName, $input['value']);
+    $value = $input['value'] ?? '';

-    return SlimUtils::renderJSON($response, ['value' => SystemConfig::getValue($configName)]);
+    // Never overwrite a password with an empty value
+    if (SystemConfig::getConfigItem($configName)->getType() === 'password' && empty($value)) {
+        return SlimUtils::renderJSON($response, ['value' => '']);
+    }
+
+    SystemConfig::setValue($configName, $value);
+
+    return SlimUtils::renderJSON($response, ['value' => '']);


The new password-type protection behaviors in the config API are not covered by the existing Cypress test suite, but there are already tests for the /admin/api/system/config endpoint (cypress/e2e/api/private/admin/private.admin.person.initial.style.setting.spec.js). The following behaviors should be tested:

GET /admin/api/system/config/sTwoFASecretKey (or sSMTPPass) returns {value: ''} regardless of what is stored.

POST /admin/api/system/config/sTwoFASecretKey with an empty body returns {value: ''} and does NOT overwrite the existing value.

Copilot · 2026-03-07T17:39:50Z

src/admin/routes/api/system/system-config.php

+    SystemConfig::setValue($configName, $value);
+
+    return SlimUtils::renderJSON($response, ['value' => '']);


The setConfigValueByNameAPI function now unconditionally returns {value: ''} for all successful saves (both password and non-password types). Previously, it returned the actually saved value via SystemConfig::getValue($configName). This is a subtle breaking change in the API contract for non-password types. While no current callers appear to use the returned value, external API clients or future code might rely on the POST response containing the saved value. Consider returning the saved value for non-password types while continuing to return {value: ''} only for password types.

Copilot · 2026-03-07T17:39:50Z

src/ChurchCRM/dto/SystemConfig.php

+                // Add a generate button for the 2FA encryption key
+                if ($settingName === 'sTwoFASecretKey') {
+                    $entry['generate'] = true;
+                }


The getSettingsConfig method in SystemConfig is a generic utility for building settings panel configurations, but it now contains a hardcoded special case for sTwoFASecretKey to inject generate: true. This couples a generic utility to a specific setting name, making it harder to extend (any future password-type setting that needs a generate button would require another special case here).

A cleaner approach would be to either: (a) add a generate metadata flag to the ConfigItem constructor/definition so the getSettingsConfig loop can read it generically, or (b) allow the caller (UserService::getUserSettingsConfig) to inject the generate property after getting the base config. This is a design concern, not a blocking issue.

Copilot · 2026-03-07T17:39:50Z

src/admin/routes/api/system/system-config.php

-    return SlimUtils::renderJSON($response, ['value' => SystemConfig::getValue($args['configName'])]);
+    $configName = $args['configName'];
+    // Never return password values to the browser
+    if (SystemConfig::getConfigItem($configName)->getType() === 'password') {


In both getConfigValueByNameAPI and setConfigValueByNameAPI, SystemConfig::getConfigItem($configName) is called and its return value is immediately dereferenced with ->getType() without any null check.

getConfigItem() returns self::$configs[$name], which is null when $configName is not a valid config key. This will cause a fatal PHP error (TypeError: Cannot call method on null) at runtime rather than returning a graceful 400/404 HTTP response.

Before this PR, the flow would call SystemConfig::getValue() directly (which throws an \Exception with a meaningful message), and any exception handling in the Slim error middleware would produce a proper error response. Now, the new getType() check introduces a crash path.

A null check before calling ->getType() should be added, returning a 400 or 404 error response if the config item is not found, consistent with how renderErrorJSON is used elsewhere in the codebase.

- Auto-generate sTwoFASecretKey in LoadConfigs.php when 2FA is enabled/required and no key exists — admins never need to touch it - Remove sTwoFASecretKey from all admin UI surfaces (Quick Settings panel, System Settings categories, generate-button hint) - sTwoFASecretKey stays in SystemConfig/DB for persistence but is invisible to the admin - Default bEnable2FA to off — admin must explicitly enable 2FA - Remove now-unreachable AdminService warning for missing secret key - Delete unsupported-2fa.php template; replace with a dashboard redirect via SlimUtils::renderRedirect when 2FA is not supported - Fix SettingsUser.php Cancel button: was navigating to /admin dashboard, now correctly links to /admin/system/users Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Previously, users with valid credentials were hard-denied ('Invalid login or password') when bRequire2FA was enabled but they had not enrolled in 2FA — giving them no way to enroll and locking everyone out of the system. Now follows the same pattern as forced password change: - Login succeeds normally - Every subsequent request redirects to the 2FA enrollment page - Redirect is suppressed when already on the enrollment page - Once enrolled, the redirect check passes and normal access resumes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

2FA enrollment no longer requires admin opt-in. Every user can always self-enroll via their profile menu. The only admin control is bRequire2FA which mandates enrollment for all users. Changes: - Remove bEnable2FA from SystemConfig, UserService (Quick Settings), and all auth logic — enrollment is always open - getIsTwoFactorAuthSupported() removed — no longer conditional; 2FA routes and the header menu item are always available - LocalAuthentication.authenticate() now redirects to 2FA verification whenever the user has enrolled, regardless of system flags - LoadConfigs.php auto-generates sTwoFASecretKey unconditionally on boot - Header.php: "Manage 2FA" always shown, LocalAuthentication import removed - user-current.php: manage2fa() guard and SlimUtils import removed - users.php: 2FA column always shown; table UX improved (badges, no Total Logins column, dropdown-menu-right alignment) - Add 7.0.2-2fa.sql migration to delete bEnable2FA from config_cfg - Register migration in upgrade.json Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 7, 2026 17:31

DawoudIO requested a review from a team as a code owner March 7, 2026 17:31

DawoudIO requested review from DAcodedBEAT, MrClever, bigtigerku, grayeul and respencer and removed request for a team March 7, 2026 17:31

Copilot started reviewing on behalf of DawoudIO March 7, 2026 17:31 View session

fix: align Quick Settings panel title with button label

60e8253

The panel was titled "User Settings" while the toggle button reads "Quick Settings" — update the panel title to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI reviewed Mar 7, 2026

View reviewed changes

DawoudIO and others added 2 commits March 7, 2026 09:57

Update messages.po

195ea7b

DawoudIO added the Security label Mar 7, 2026

DawoudIO added this to the 7.0.2 milestone Mar 7, 2026

DawoudIO and others added 3 commits March 7, 2026 10:13

Update cypress-testing.md

0a80d3c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: consolidate 2FA settings into User Settings page#8210

feat: consolidate 2FA settings into User Settings page#8210
DawoudIO wants to merge 7 commits intomasterfrom
feature/2fa-settings-user-panel

DawoudIO commented Mar 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		SystemConfig::setValue($configName, $value);

		return SlimUtils::renderJSON($response, ['value' => '']);

Uh oh!

Conversation

DawoudIO commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Core Authentication & Config

API & Routes

UI

Database & Docs

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DawoudIO commented Mar 7, 2026 •

edited

Loading